-
Notifications
You must be signed in to change notification settings - Fork 85
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/belongs to many support #97
base: master
Are you sure you want to change the base?
Feature/belongs to many support #97
Conversation
nice! I will review and merge tomorrow |
I am using this package in my commercial project so it is important to me to keep this package clean. I see that you mixed 4 things in this PR :
can you create a distinct PR for each of them ?
currently you are using :
|
Yes i separate PRs.
|
I think your solution for 1. is not good enough, because when you need, for example, to filter by JSON your solution would now work.... with
it would be :
|
it turns out it will not be compatible with useTableAlias, and for support useFullPathTableAlias
is it OK for you? |
But in my solution everything will work also we will not be connected with the real name of the table, and suddenly we’ll imagine the table name changes and we don’t need to change the code, I think this is more correct. |
yeah, whereRaw is very specific, I would definitely go with this interface :
but I am not sure which solution is the best for above case : this one is valid for now and it can be merged:
but later I will might go with more elegant solution, maybe something like this :
|
I have to hardcode this prefix. I see unobvious behavior in the future and possible conflicts, but in my decision it remains entirely under the control of the developer.
|
in your PR just use this interface
and then this would work :
this is just for a consideration for the future, not confirmed solution, and not for this PR :
but this is just an upgrade, with that upgrade the old solution would also work :
|
Yes, I understand, I just now need to dynamically set the table, because I am doing a very universal API. therefore, I want to implement support for this functionality. |
1676c2b
to
e9edd6f
Compare
I can't separate start of morph relation because i also use it for testing InvalidRelation exception like you tested belongToMany in current master. |
any progress on this? thanks @tonyfulls845 @fico7489 |
No description provided.